refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's `Psbt::sign` instead. #235

oleonardolima · 2025-05-19T13:44:11Z

fixes #70

Description

It's a work in progress to remove signers from Wallet and all related APIs. Users should use their own signer implementation, which needs to implement the GetKey trait from rust-bitcoin in order to rely on the Psbt::sign method.

Adds a reference implementation of a new SignerWrapper type as a test utility, which is currently being used on existing tests, instead of Wallet::sign.

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

coveralls · 2025-06-03T21:22:05Z

Pull Request Test Coverage Report for Build 18367106306

Details

106 of 120 (88.33%) changed or added relevant lines in 3 files are covered.
256 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-3.1%) to 81.735%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/wallet/mod.rs	13	15	86.67%
src/test_utils.rs	77	89	86.52%

Files with Coverage Reduction	New Missed Lines	%
src/wallet/mod.rs	1	81.9%
src/test_utils.rs	3	94.21%
src/wallet/params.rs	7	72.38%
src/wallet/signer.rs	245	32.41%

Totals
Change from base Build 18318022693:	-3.1%
Covered Lines:	6672
Relevant Lines:	8163

💛 - Coveralls

thunderbiscuit · 2025-06-04T13:05:38Z

It's funny I was looking at the diff here when I got the notification that you had requested a review! I was like how does he know I'm looking at the files right now??? haha.

Concept ACK. The Wallet type is so much, but signing the PSBTs is really something separate. Added bonus this signing workflow actually already exists and makes sense as part of the rust-bitcoin ecosystem. Let's go!

My only comment is that because this is breaking, even once it's all good to go you might end up having to keep it fresh and rebase for a while because we might not be ready to merge breaking/3.0 features on master before mid-August (as per previous discussions, but that could change of course).

ValuedMammal · 2025-06-12T23:26:32Z

These are some of my high-level impressions, granted the details may be subject to change.

I hesitate about keeping the sign/finalize flow in the example crates while SignerWrapper is still in its infancy. If the idea of the issue is to simultaneously "provide a new way to sign", can that be achieved with the async-hwi example feat: example using hwi signing with #206 ?
Some tests that depend on the existence of signers can be changed to either sign using SignerWrapper, or else simulate signing by adding a fake sig / satisfaction weight. However my opinion is that any tests that were specifically written to test the old signing code (signer.rs) should probably just be removed.
finalize_psbt is kind of a mess. SignOptions::assume_height is arguably not needed because After and Older are basically obsolete, but correct me if I'm wrong. The only possible error in finalize_psbt is SignerError::IndexOutOfRange - we can make a new error type for this. Have a structured IndexOutOfBoundsError type #267
Make signer module pub(crate) and un-expose SignOptions, SignerError. For the moment SignersContainer is still being used.
In terms of review it would be great if this PR was just a net-removal of code, and then have a potential followup with the experimental SignerWrapper, but I understand the motivation for having it here.

ValuedMammal · 2025-06-14T22:59:46Z

src/wallet/mod.rs

+            .extract_policy(
+                &SignersContainer::default(),
+                BuildSatisfaction::None,
+                &self.secp,
+            )?


Following the idea of SignersContainer::default, in make_generic_signature we can set the contribution to None and remove the signers parameter from the function, and any function that calls it. The rationale is that a wallet with no signers won't be contributing a signature to the overall satisfaction.

This is a larger refactor but has the benefit of ultimately decoupling the signer module.

bdk_wallet/wallet/src/descriptor/policy.rs

Lines 763 to 779 in 00dafe7

fn make_generic_signature<M: Fn() -> SatisfiableItem, F: Fn(&Psbt) -> bool>(

key: &DescriptorPublicKey,

signers: &SignersContainer,

build_sat: BuildSatisfaction,

secp: &SecpCtx,

make_policy: M,

find_sig: F,

) -> Policy {

let mut policy: Policy = make_policy().into();

policy.contribution = if signers.find(signer_id(key, secp)).is_some() {

Satisfaction::Complete {

condition: Default::default(),

}

} else {

Satisfaction::None

};

Yes, I agree with the rationale and it's better than having to keep the SignersContainers only for it's default implementation.

I've started playing with this, you can take a look here: oleonardolima@e9f6b0a. I'm taking a look on the failing tests, to check if the breaking changes are no big deal.

ValuedMammal · 2025-08-21T15:06:31Z

I thought about it and I don't mind having SignerWrapper as a test utility if it can be used in tests and examples.

…etKey` e2d934e feat: impl `GetKey` for `KeyMap` and `DescriptorSecretKey` (Leonardo Lima) 6f345d7 refactor(keymap): create `KeyMap` type (Tobin C. Harding) 131cf01 chore(deps): bump `bitcoin` to `0.32.6` (Leonardo Lima) Pull request description: fixes #709 ### Description I've built upon Tobin's work in #765 (see #765 (comment)) to implement the `GetKey` trait for `KeyMap`, a feature that will enable us to entirely remove the signers API from `bdk_wallet`, see: bitcoindevkit/bdk_wallet#70 and bitcoindevkit/bdk_wallet#235. ### Notes to the reviewers I followed some of Poelstra's previous comments to reduce the number of required matching branches, and also some VM's suggestions on implementing the main `GetKey` logic for `DescriptorSecretKey` instead. Let me know if you have any other suggestions or comments. ### Changelog notice ```md # Added: - implemented `GetKey` trait for `KeyMap` - implemented `GetKey` trait for `DescriptorSecretKey` # Changed - moved `KeyMap` to it's own `key_map` module, replacing the `BTreeMap` alias. ``` ACKs for top commit: apoelstra: ACK e2d934e; successfully ran local tests Tree-SHA512: 85168f3d6de0527d077674ff4e988ecda9f6237ae1c15018a1cfeb5760fa5a14b700b58a9e78d79b15cffba12b3f1e8de3a5e82c0822a0506aa7fa926bf8a7d8

ItoroD · 2025-09-26T16:22:48Z

@oleonardolima I have ran some of the test. I am particularly interested in the psbt.sign part. The result after running test seems psbt.sign(my_signer, secp) is working at this point. I can see that the psbt got signed 👍

Is SignerWrapper going to remain a test utility? Or will it develop into some sort of default signer? One which we can always just create an instance of and pass it to the psbt sign method.

Before user just needed to pass the psbt to wallet.sign() without worrying about signer implementation and all. (sorry to drag you back if this had been talked about already)

…`KeyMap` and `DescriptorSecretKey` e2d934eb9530ffac3a26cb3c822dd5ed9bc29a85 feat: impl `GetKey` for `KeyMap` and `DescriptorSecretKey` (Leonardo Lima) 6f345d75b03d49a1e606d161656067a7f8331cf2 refactor(keymap): create `KeyMap` type (Tobin C. Harding) 131cf01f318fa9ba98c03da8ea9bab62ce7d44eb chore(deps): bump `bitcoin` to `0.32.6` (Leonardo Lima) Pull request description: fixes rust-bitcoin/rust-miniscript#709 ### Description I've built upon Tobin's work in #765 (see rust-bitcoin/rust-miniscript#765 (comment)) to implement the `GetKey` trait for `KeyMap`, a feature that will enable us to entirely remove the signers API from `bdk_wallet`, see: bitcoindevkit/bdk_wallet#70 and bitcoindevkit/bdk_wallet#235. ### Notes to the reviewers I followed some of Poelstra's previous comments to reduce the number of required matching branches, and also some VM's suggestions on implementing the main `GetKey` logic for `DescriptorSecretKey` instead. Let me know if you have any other suggestions or comments. ### Changelog notice ```md # Added: - implemented `GetKey` trait for `KeyMap` - implemented `GetKey` trait for `DescriptorSecretKey` # Changed - moved `KeyMap` to it's own `key_map` module, replacing the `BTreeMap` alias. ``` ACKs for top commit: apoelstra: ACK e2d934eb9530ffac3a26cb3c822dd5ed9bc29a85; successfully ran local tests Tree-SHA512: 85168f3d6de0527d077674ff4e988ecda9f6237ae1c15018a1cfeb5760fa5a14b700b58a9e78d79b15cffba12b3f1e8de3a5e82c0822a0506aa7fa926bf8a7d8

oleonardolima · 2025-09-29T00:22:31Z

Thanks for taking a look into this one, I still need to rebase and update some parts of it after backporting rust-miniscript#851.

Is SignerWrapper going to remain a test utility? Or will it develop into some sort of default signer? One which we can always just create an instance of and pass it to the psbt sign method.

No, the idea is to use directly the implementation from rust-bitcoin/rust-miniscript. Yes, if you need to just wrap it for some reasone you'll need to create an isntance of it an pass for each test, instead of having a re-implementation of it in the test-utils.

Before user just needed to pass the psbt to wallet.sign() without worrying about signer implementation and all. (sorry to drag you back if this had been talked about already)

It'd still be basically the same, the user won't need to care about the implementation of signing itself, just to have a type that implements a GetKey, if for some reason the user needs to use a different type that is not covered in rust-miniscript/rust-bitcoin, then it'll need to implement it's own GetKey, extracting the required key properly.

ItoroD · 2025-09-29T09:09:06Z

Thanks for taking a look into this one, I still need to rebase and update some parts of it after backporting rust-miniscript#851.

Is SignerWrapper going to remain a test utility? Or will it develop into some sort of default signer? One which we can always just create an instance of and pass it to the psbt sign method.

No, the idea is to use directly the implementation from rust-bitcoin/rust-miniscript. Yes, if you need to just wrap it for some reasone you'll need to create an isntance of it an pass for each test, instead of having a re-implementation of it in the test-utils.

Before user just needed to pass the psbt to wallet.sign() without worrying about signer implementation and all. (sorry to drag you back if this had been talked about already)

It'd still be basically the same, the user won't need to care about the implementation of signing itself, just to have a type that implements a GetKey, if for some reason the user needs to use a different type that is not covered in rust-miniscript/rust-bitcoin, then it'll need to implement it's own GetKey, extracting the required key properly.

Ah! Thanks for explanation!

So some of wrappers were sort of place holders for the updates now done in 851 rust-miniscript. This Pr was done before that was merged. Makes sense.

Look forward to seeing the update on here!

- adds a new `SignerWrapper` for `KeyMap` as a common example utility. - adds an implementation of `GetKey` trait for `SignerWrapper`, in order to retrieve the private key for software signers, and successfully use `Psbt::sign` method. - the `SignerWrapper` is necessary temporarily, in order to update the existing examples to use `Psbt::sign` instead of `Wallet::sign`, as the native implementation of `GetKey` for the `KeyMap` type has not been released yet in `rust-miniscript.

- updates `example_wallet_{electrum|esplora}` examples to use the `SignerWrapper` implementation from common module. - update examples to parse the used descriptors into a `KeyMap`, and using `SignerWrapper` to sign the transaction with `Psbt::sign` method, it still uses the `Wallet::finalize_psbt` though.

- adds a new `SignerWrapper` for `KeyMap` as a test utility, with new methods: `get_wallet_signer` and `get_wallet_signer_single`. - adds an implementation of `GetKey` trait for `SignerWrapper`, in order to retrieve the private key for software signers, and successfully use `Psbt::sign` method. - the new methods added are necessary, in order to update the existing tests to use `Psbt::sign` instead of `Wallet::sign`, as it further allows the signing APIs and related types to be fully removed.

- updates the existing tests which use `Wallet::sign` API, to get the signer implementation with: `get_wallet_signer_single` or `get_wallet_signer`. - FIXME: there are two tests which requires further refactoring, discussion and update, being: `test_psbt_sign_with_finalized` and `test_psbt_multiple_internalkey_signers`.

- updates the existing tests that uses the `Wallet::sign` API, to gethe signer implementation from test utilities, either: `get_wallet_signer_single` or `get_wallet_signer`. - there are a few tests which needs further refactoring, as they rely on the use of `Wallet::finalize_psbt`.

- use default empty `SignersContainer::new()` on both wallet `create_with_params` and `load_with_params`. - remove `add_signer`, `set_keymap` and `get_signers` methods. - updates `FullyNodedExport::export_wallet` to export public descriptors only, and updates it's tests accordingly. - remove test assertions for signers/keymaps creation/load. - remove both fields `signers` and `change_signers` from `Wallet`. - all usage of signers for `extract_policy` fns in `create_tx` and `policies` methods now rely on the `SignersContainer::default()`.

oleonardolima self-assigned this May 19, 2025

oleonardolima added help wanted Extra attention is needed api A breaking API change labels May 19, 2025

oleonardolima added this to BDK Wallet May 19, 2025

notmandatory moved this to In Progress in BDK Wallet May 20, 2025

oleonardolima added this to the Wallet 3.0.0 milestone May 27, 2025

oleonardolima force-pushed the refactor/remove-signers branch from 6249eac to b819375 Compare May 30, 2025 14:46

luisschwab self-requested a review June 1, 2025 18:51

oleonardolima force-pushed the refactor/remove-signers branch 2 times, most recently from 5eccb29 to e8d9c17 Compare June 2, 2025 17:57

oleonardolima mentioned this pull request Jun 2, 2025

Implement GetKey for KeyMap rust-bitcoin/rust-miniscript#765

Open

oleonardolima force-pushed the refactor/remove-signers branch 6 times, most recently from d41948b to 6b60d04 Compare June 3, 2025 14:04

oleonardolima force-pushed the refactor/remove-signers branch from 6b60d04 to 399ad6f Compare June 4, 2025 12:55

oleonardolima changed the title ~~wip(refactor(wallet))!: remove signers and it's APIs, relying on rust-bitcoin's Psbt::sign instead.~~ refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's Psbt::sign instead. Jun 4, 2025

oleonardolima requested review from notmandatory, thunderbiscuit and ValuedMammal June 4, 2025 12:58

This was referenced Jun 5, 2025

Sanity check PSBT before signing #52

Open

feat(signer): merge internal and external signer containers #251

Closed

ValuedMammal reviewed Jun 14, 2025

View reviewed changes

oleonardolima force-pushed the refactor/remove-signers branch from 399ad6f to c60cdd2 Compare July 4, 2025 20:24

oleonardolima force-pushed the refactor/remove-signers branch from c60cdd2 to 498757f Compare August 20, 2025 19:30

oleonardolima mentioned this pull request Aug 20, 2025

[DRAFT] Clean up outdated signing code #301

Closed

2 tasks

oleonardolima mentioned this pull request Aug 22, 2025

feat: implement GetKey for KeyMap and DescriptorSecretKey rust-bitcoin/rust-miniscript#851

Merged

ItoroD mentioned this pull request Sep 29, 2025

Add Psbt::sign API to Psbt type bitcoindevkit/bdk-ffi#866

Open

oleonardolima force-pushed the refactor/remove-signers branch from 498757f to 30280fb Compare October 8, 2025 06:10

tnull mentioned this pull request Oct 8, 2025

Migrate from deprecated signer module to bitcoin::psbt in ldk-node lightningdevkit/ldk-node#654

Open

oleonardolima force-pushed the refactor/remove-signers branch from 30280fb to 6def19f Compare October 9, 2025 06:03

oleonardolima added 6 commits October 9, 2025 03:04

oleonardolima force-pushed the refactor/remove-signers branch from 6def19f to e7a8a1a Compare October 9, 2025 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's `Psbt::sign` instead. #235

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's `Psbt::sign` instead. #235

Uh oh!

oleonardolima commented May 19, 2025 •

edited

Loading

Uh oh!

coveralls commented Jun 3, 2025 •

edited

Loading

Uh oh!

thunderbiscuit commented Jun 4, 2025

Uh oh!

ValuedMammal commented Jun 12, 2025

Uh oh!

ValuedMammal Jun 14, 2025

Uh oh!

oleonardolima Jun 17, 2025

Uh oh!

ValuedMammal commented Aug 21, 2025

Uh oh!

ItoroD commented Sep 26, 2025

Uh oh!

oleonardolima commented Sep 29, 2025

Uh oh!

ItoroD commented Sep 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

	fn make_generic_signature<M: Fn() -> SatisfiableItem, F: Fn(&Psbt) -> bool>(
	key: &DescriptorPublicKey,
	signers: &SignersContainer,
	build_sat: BuildSatisfaction,
	secp: &SecpCtx,
	make_policy: M,
	find_sig: F,
	) -> Policy {
	let mut policy: Policy = make_policy().into();

	policy.contribution = if signers.find(signer_id(key, secp)).is_some() {
	Satisfaction::Complete {
	condition: Default::default(),
	}
	} else {
	Satisfaction::None
	};

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's Psbt::sign instead. #235

Are you sure you want to change the base?

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's Psbt::sign instead. #235

Uh oh!

Conversation

oleonardolima commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

coveralls commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18367106306

Details

💛 - Coveralls

Uh oh!

thunderbiscuit commented Jun 4, 2025

Uh oh!

ValuedMammal commented Jun 12, 2025

Uh oh!

ValuedMammal Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Aug 21, 2025

Uh oh!

ItoroD commented Sep 26, 2025

Uh oh!

oleonardolima commented Sep 29, 2025

Uh oh!

ItoroD commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's `Psbt::sign` instead. #235

refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's `Psbt::sign` instead. #235

oleonardolima commented May 19, 2025 •

edited

Loading

coveralls commented Jun 3, 2025 •

edited

Loading

ItoroD commented Sep 29, 2025 •

edited

Loading